BZ#145848583 - Introduce service tag filtering #840
Conversation
AllenBW
force-pushed
the
BZ/#145848583-filter-service-by-tag
branch
from
0b86615
to
79e891c
Compare
|
Looking good so far, I'll do a final review once WIP is lifted. |
AllenBW
force-pushed
the
BZ/#145848583-filter-service-by-tag
branch
from
576c717
to
21da3ea
Compare
|
@chriskacerguis This pr is done, but there are two api bugs that I would like to be sorted prior to merging this |
AllenBW
changed the title
|
@AllenBW Can you please make BZs for those issues and put the links in the PR comments? |
|
@chriskacerguis yeah! totally did, travis passed, but its not being reported, any idea whats up? |
|
Looks like it passed: https://travis-ci.org/ManageIQ/manageiq-ui-service/builds/252562261 Do you want me to merge or wait for those bugs to pass? |
|
@chriskacerguis thats a gooooooooooood question.... i know one is on dev presently, the other should be soonish... without them filtering by tags still works... its just breakable :P As there is no more work to do here and bz are made... i think merging is ok to do |
|
@ManageIQ/core-ux for your review. /cc @Loicavenel |
|
I anticipate one feedback might be "ew, those tags are long and silly" buttttttt this the pf-ang lib doesn't support an tag value alias, there might be another way to manage this, i have an idea, working it, but the need for the ability to display a tag alias (rather than the search value) has also been expressed here https://bugzilla.redhat.com/show_bug.cgi?id=1470868 |
|
|
|
@AllenBW since we are still waiting for UX review, I would say let's just add it to this one. |
|
I will leave Serena reviewing this one. But, I see 2 issues:
|
|
@AllenBW definitely think that this is a useful capability. Colleen and I worked on this design, and would like this to be implemented:
Upon selecting to filter by tag [1], then there will be two components added: Both of those components should use the typeahead dialog select, as you did here: #821 When the value [3] is designated, a filter chip [4] should be added in the format: Does this seem achievable? Let me know your thoughts @AllenBW @Loicavenel ! |
|
@serenamarie125 yeah, I don't recall doing this in the past. |
|
Thanks for the supa fast reply @jeff-phillips-18 ❤️ 🙇♀️ (I'm not confused, its the closet reaction github provides to a sad face) @serenamarie125 given that this isn't present functionality and will require a nontrivial amounta effort to get in, you down with adding the present functionality as is with the pt to circle back around and "fix" when the desired look/feel exists? |
|
@serenamarie125 would @AllenBW suggestion (#840 (comment)) be ok with you? /cc @Loicavenel |
|
@AllenBW I think there are usability issues with the current PR, and don't think that it should be added. Although PF doesn't currently support it, I would think that you could either enhance the component and contribute it back, or just tailor in the SUI and contribute back at a later date? |
|
+1 |
|
@serenamarie125 if we're to continue to use patternfly, this component would have to be added to patternfly, not something we can tailor in the sui, I'll keep this pr out here and mark the pt as blocked |
|
@AllenBW i don't think this should block SUI implementing a usable solution. PatternFly is an OpenSource community, please feel free to contribute what you need to Angular PatternFly. |
|
@serenamarie125 it was my understanding that this pr couldn't go through until it met the mock above? that can't happen until the additions are made to the repo, that we have to hold off on this pr until those changes are present means this pr is blocked 👍 🌮 😿 |
|
@AllenBW @chriskacerguis If this new "UI fonction" is not available today, why can you make it? |
|
Woaaaaaah @Loicavenel I never said we couldn't add it to patternfly, apologies if I wasn't clear, the functionality is not in ang-pf at this very moment, which means this pr is blocked 😺 |
|
@AllenBW ok, so we can move on this PR with doing it in Partnerfly or doing it our own way close to Partnerfly and someone will move it to partnerfly later |
|
@Loicavenel sounds good to me! |
|
This pull request is not mergeable. Please rebase and repush. |
AllenBW
force-pushed
the
BZ/#145848583-filter-service-by-tag
branch
from
21da3ea
to
431a949
Compare
arrow-body-style would say its an expression no need for return and no-confusing-arrow would say its a confusing expression it needs a return, opting for the less invasive of the two, expressions should be encapsulated in params
Right now, api doesn't handle anding together multiple tag filters, when it supports, filtering by multiple tags will work outta da box
Need to rework polling on this, as it stands it a flashback whenever conents are filtered or sorted
We stop polling whenever a new request for resources has been made (or the component is destroyed) After successfully grabbing these new resources, we begin polling on the successful call This avoids the previous issue of stale polling
AllenBW
force-pushed
the
BZ/#145848583-filter-service-by-tag
branch
from
431a949
to
1c9844d
Compare
|
Checked commits AllenBW/manageiq-ui-service@056ecd5~...1c9844d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@chriskacerguis how did this get merged? Both @Loicavenel and I requested a change |
|
Whoops. My bad. You did. I'll back it out |
|
Thanks @chriskacerguis |
Update, this pr is done, but there are two api bugs that I would like to be sorted prior to merging this in (detailed below)
Why are we wipping? present issues (and prs solving when they are up)All of the following blockers are resolved:polling results in a call back sorta effect whenever filter or sorting is applied, need to rework this immediatelyWe stop polling whenever a new request for resources has been made, ie a filter or sorting event (or the component is destroyed)
After successfully grabbing these new resources, we begin polling on the successful call
This avoids the previous issue of stale polling
api anding of multiple filters does not quite work as expected, issue has been identified and is being looked at (not an sui issue)as was pointed out in the bz, this is actually expected behavior, we are looking for services with only the tags added not including the tags
https://bugzilla.redhat.com/show_bug.cgi?id=1470251
api throws error when changing sort direction of filtered tagshttps://bugzilla.redhat.com/show_bug.cgi?id=1470260
Force ascending order manageiq#15559
No more polling artifact <3 <3 <3
What filtering by tag looks like, also what this polling artifact bug looks like :(